-
-
Notifications
You must be signed in to change notification settings - Fork 408
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate Route Render APIs #418
Conversation
I'm a HUGE 👍 on this (though we do have some legacy features that rely on route rendering). This really simplifies the route/template and rendering relationship across the broader ecosystem. Especially when advising older projects that rely heavily on From a migration strategy I think some migration guides around common scenarios which from experience route rendering can usually be replaced by one of a few strategies:
|
…ecate-route-render-methods.md (#422)
@chadhietala Rendered link not working any more. |
We are not deprecating |
I quickly grep’d the Heroku Dashboard code base to see how we’d be impacted by this. Turns out we use The vast majority deal with rendering ancillary UI into top-level outlets (e.g. top-nav, action-bar). Using wormholes seems like an appropriate migration path for these. The other usages are to switch between different templates for an error route. Presumably the appropriate migration path for this is to turn those templates into components. |
Here's a scenario that uses |
In one of my projects I use I tried to visualize the situation in the image below. I hope it makes sense in combination with the code snippets. // router
this.route('authenticated', { path: '' }, function() {
this.route('campaigns', { resetNamespace: true }, function() {
this.route('campaign', { path: ':campaign_id' }, function() {
this.route('messages', function() {
this.route('message', { path: ':message_id' });
});
});
});
}); // campaign.messages
renderTemplate() {
this.render({ into: 'authenticated' });
} |
I use |
We make a lot of use of render and renderTemplate as well, to render templates in different places. For example dialogs and fullscreen components that can’t be part of the html tree but have to be below it instead. We have a few named outlets for this in application.hbs. I don’t see how this can be replaced with components, as there isn’t just a single one for each outlet but there is a number of components that has to be rendered there depending on the route. The only way for us to solve this would be to allow these things to be rendered in the tree css-wise I guess. In any case this would leave us with a huge architectural question. |
@rmachielse You would use something like https://github.com/ef4/ember-elsewhere and create a component that has multiple components in it's template (for the multiple component scenario you mentioned). |
@knownasilya yep, something like that and ember-wormholes are rightfully alternatives. |
Yeah maybe that is true about it being a type of work around, although things like ember-elsewhere are using public API (see this merged RFC #287) so they are very lightweight. I do like the how ember-component-routes addon ("routable components" experiment) handles this, feels very clean: https://wongpeiyi.github.io/ember-component-routes/#/rendering. e.g. import { ComponentRoute } from 'ember-component-routes';
export default ComponentRoute.extend({
renderComponents() {
this.renderComponent('app-menu', { outlet: 'sidebar' });
this.renderComponent('post-page', { into: 'parent-page' });
}
}); Where So maybe an alternative API should be proposed, to at least allow experimentation like above. |
That one looks cleaner indeed. However, as long as ember has controllers and routes, |
@rmachielse what's the benefit of using fwiw I'm very much in favor of deprecating this as removing the api encourages to use declarative templates that tell a developer where stuff is rendered by just looking at the template files rather than digging through the routes. |
@LevelbossMike the separation of concerns is the most valuable thing to me. I think the decision of where to render a template should not be a responsibility of the template itself. Also in our case it allows us to reuse templates, without having to change anything in the template itself. |
If this functionality is considered to be too far away from core principles, could it be extracted to an addon? Not one that extends support temporarily but one that can be maintained for longer time? Or does it depend on internal api's too much? |
Deprecation doesn't mean it will be removed right away, so it should be around until Ember 4.0, which is still probably a year away most likely (imo). |
Understood, but I would like a future-proof solution. |
I have updated the RFC with clearer examples on how to migrate. @rmachielse: #287 introduced a public API to do what ember-elsewhere / ember-wormhole do. It was created specifically for the cases where you do need to escape the existing DOM hierarchy and render somewhere else. This is great for things like modals, adding to navigation depending on downstream logic, etc. In the cases where you do need access a controller for a specific template and don't want to refactor to a component you can inject controllers onto other objects. The opinion of the core team is that components are the way templates / functionality should be shared and reused. @Gaurav0 |
@chadhietala Thanks for adding more examples! I can see how In your hierarchy escaping example, would it be possible to reliably render into the It feels a bit weird to replace the contents of the wrapping element, as it feels like it would remove the outlet. (Please forgive me my ignorance about the details of how |
@chadhietala any public way to force route template rerender without |
@lifeart have you seen emberjs/ember.js#17000 and emberjs/ember.js#17001 ? |
@chadhietala of course! I made ember-ast-hot-reload addon. And it allow users to hot-reload components (using PoC from ember tests) and route templates, using Code examples above don't deal with route level templates. |
@Kerrick and @Kilowhisky please take a look at the follow example. I believe both of the concerns are around escaping the hierarchy and I believe this can be solved with |
I can't say I'm a fan of removing this, others have already expressed how they use it and we use it in a similar way to @Kilowhisky . It works well for us, it is simple to reason about and every other alternative pattern we have tried is a PITA to reason about and has worse rendering performance. I guess we will have to take a stab at the alternatives again. |
I’d like to understand this better. I find the alternatives here massively easier to reason about, what makes them seem overly complicated in your scenarios? I’m also curious how the proposed alternatives would have any different performance at all. |
Maybe I should have tried the in-element alternative before I posted the above. We had not tried that pattern, we have been using ember for so long that we settled into our current patterns a couple years ago. I am still finding renderTemplate easier to keep a mental model of but that could be because I am used to our projects being organized that way. I built two twiddles of the same dummy app, one using renderTemplate one using the suggested Maybe if I understood why the change was necessary it would help me be more excited about it. I'm not seeing many other comments saying they want the change, of course the core team probably has more channels than these comments to base a decision on. Using renderTemplate Using |
Following @nullrocket's example, I tried to implement my What am I missing (or doing wrong)?
|
@benedikt as far as I understand |
@LevelbossMike Aah, thanks! That explains my issue. Looks like this will work for my use-case indeed. 👍 |
|
||
# How We Teach This | ||
|
||
This has not been a mainline API for quite some time now. The guides do not mention this functionality at all. It is likely that the majority of Ember applications do not use these APIs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has not been a mainline API for quite some time now.
I think this was supposed to read "This API has not existed for very long"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm referring to the fact that renderTemplate
and the associated APIs do not and have not played a predominate role in the programming model for quite some time now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API existed in Ember 1.0, so it definitely "has existed for very long". :)
I'd like to suggest a first draft of the steps we need to take to implement this RFC (mostly cribbed from best practice, I don't think we have any of this written down):
|
Add more process context.
@mixonic Can you review the latest changes and confirm that they address your process concerns? |
|
||
# How We Teach This | ||
|
||
These APIs not been a mainline API for quite some time now. The guides briefly mention this functionality. In those cases we should mirgate the guides should link to the `{{in-element}}` documentation and the component documentation. The above "Transition Path" will serve as the deprecation guide. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These APIs not been a mainline API for quite some time now. The guides briefly mention this functionality. In those cases we should mirgate the guides should link to the `{{in-element}}` documentation and the component documentation. The above "Transition Path" will serve as the deprecation guide. | |
These APIs not been a mainline API for quite some time now. The guides briefly mention this functionality. In those cases we should migrate the guides should link to the `{{in-element}}` documentation and the component documentation. The above "Transition Path" will serve as the deprecation guide. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it would be helpful for this sentence to state its subject. I think @chadhietala's wording in his response to be above was just better:
renderTemplate
and its associated APIs do not and have not played a predominate role in Ember's common programming model for quite some time. The guides only briefly mention this functionality. In those guides cases that are documented (maybe should link to them here) the guides should link to the{{in-element}}
documentation and component documentation as is appropriate.The "Transition Path" section above will serve as a first draft of the deprecation guide.
|
||
# Role Out Plan | ||
|
||
Prior to adding the deprecation we must first do the following items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prior to adding the deprecation we must first do the following items | |
We must first do the following items prior to adding the deprecation: |
|
||
## Unresolved questions | ||
|
||
Optional, but suggested for first drafts. What parts of the design are still |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional, but suggested for first drafts. What parts of the design are still |
## Unresolved questions | ||
|
||
Optional, but suggested for first drafts. What parts of the design are still | ||
TBD? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBD? |
@chadhietala Thank you for the continued cleanup on this after it had theoretically already been approved for merge :-/ Please consider my previous comment about next steps addressed by your changes. I think you already plan to raise the confusing state of the Thanks Chad. |
Yep, the confusion is cleared up on that front. |
Thanks for iterating on this @chadhietala / @mixonic! I think this is in a good place to land (only a few weeks after we meant to 😸)... |
Haha @rwjblue can you drop a link or summarize to break the suspense? |
@mixonic hmmm, I don't follow, you laid out the specifics in #418 (comment):
You were totally right, |
FWIW, @chadhietala is working on those changes over in glimmerjs/glimmer-vm#918. |
@rwjblue right, when I say:
I'm definitely talking about that
So the item I was suggesting could be added or at the least should be tracked is that glimmerjs/glimmer-vm#918 and the sibling PR to Ember should be migrating to |
To me, the target route template: |
Rendered
Closes #304.